Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Option<Vec> and Option<String> #14

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kuecks
Copy link

@kuecks kuecks commented Jun 30, 2024

The main issue with supporting these structs is that they are both 3 pointers wide, while the existing code relied on Option being precisely 1 pointer wide (and thus only worked for pointer types). To resolve this, this PR has 2 commits:

  1. Instead of using a single RustOption type to represent the C++ binding, using traits I defined different structs for the FFI representation of Option depending on T. For the old pointer types like &T and Box this is the old RustOption, but this now allows me to define new types (with a different size) to use for Option and Option

  2. Actually implement Option and Option using the indirection above

src/lib.rs Show resolved Hide resolved
Comment on lines 180 to 181
const _: () = {
impl_option_ffi! { <T: OptionPtrTarget>, usize, &'static () }
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I define the partner struct (what was RustOption). So that I don't need a unique name for each different type, the definition is tucked away in a const. This means that we can't name this type outside of this block. The rest of the commit is removing all references to RustOption outside and replacing it with <Option<T> as OptionFfi>::Target to deal with this

include/cxx.h Show resolved Hide resolved
Copy link

@capickett capickett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does is_direct correspond with needs_indirect_abi? Or are they unrelated? I was a bit confused by the naming there.

How about Option<&str>. Is that supported here? I see impl for Option<&String> but not str.

) = &sig.ret
{
write!(out, ")");
if let Some(ret) = &sig.ret {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is starting to look like it wants to be a match instead of if-let chain

}

/// SAFETY: ptr must be valid for 'a
pub unsafe fn from_raw_double_repr(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does "raw double repr" mean?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

repr is for *RustVec<T> -> Option<&Vec<T>> and double_repr is specifically for *RustVec<RustString> -> Option<&Vec<String>> (both the container and the inner type change)

However, I can also just use as _ magic casts on the pointers to get rid of both the repr and double_repr methods (just from_raw) so I think I'm gonna do that instead

@kuecks
Copy link
Author

kuecks commented Sep 3, 2024

Does is_direct correspond with needs_indirect_abi? Or are they unrelated? I was a bit confused by the naming there.

Hrmm is_direct was referring to the fact that set took in T instead of T* but now I'm not sure why I did that. It seems like I don't need to make that distinction, but now I'm not entirely sure how the code was working...

I think that the code was actually magically passing T* anyway due to calling convention so my code was luckily working. I removed the is_direct and always pass a pointer which seems more correct now (the spooky thing was that I changed the signature on the C++ side to say I pass a pointer over FFI instead of a value, but didn't change the rust side that received it. The rust side had always expected a pointer so something was strange before to say the least)

How about Option<&str>. Is that supported here? I see impl for Option<&String> but not str.

Option<&str> is not yet implemented because it is a fat pointer. Planning on doing it in a future diff though

@kuecks kuecks changed the title Supprt Option<Vec> and Option<String> Support Option<Vec> and Option<String> Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants